Automated Test: workflow-engine-stateful-detector-after #316
Conversation
… stateful detector (#80168) This adds a hook that can be implemented to produce an occurrence specific to the detector that is subclassing the StatefulDetector. Also change the signature of evaluate to return a dict keyed by groupkey instead of a list. This helps avoid the chance of duplicate results for the same group key. <!-- Describe your PR here. -->
📝 WalkthroughWalkthroughThis pull request refactors the detector evaluation system to use a group-key-indexed dictionary structure instead of lists for collecting evaluation results, introduces a stateful detector handler abstraction with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/sentry/workflow_engine/processors/detector.py (1)
226-233:⚠️ Potential issue | 🟡 MinorStale docstring: "list" should be "dict".
Line 230 says "returns a list of
DetectorEvaluationResult" but the method now returnsdict[DetectorGroupKey, DetectorEvaluationResult].Suggested fix
def evaluate( self, data_packet: DataPacket[T] ) -> dict[DetectorGroupKey, DetectorEvaluationResult]: """ - Evaluates a given data packet and returns a list of `DetectorEvaluationResult`. - There will be one result for each group key result in the packet, unless the + Evaluates a given data packet and returns a dict of `DetectorGroupKey` to `DetectorEvaluationResult`. + There will be one entry for each group key result in the packet, unless the evaluation is skipped due to various rules. """
🤖 Fix all issues with AI agents
In `@src/sentry/incidents/grouptype.py`:
- Around line 10-12: Remove or update the misleading TODO and make
MetricAlertDetectorHandler concrete by either (A) implementing the four abstract
methods declared on StatefulDetectorHandler — counter_names, get_dedupe_value,
get_group_key_values, and build_occurrence_and_event_data — with appropriate
logic or stubs that delegate to the existing MetricAlertFire behavior, or (B) if
the stateful abstraction isn't ready, change MetricAlertDetectorHandler to
inherit from the concrete DetectorHandler instead of StatefulDetectorHandler so
it no longer requires those abstract methods; ensure Detector.detector_handler
can instantiate MetricAlertDetectorHandler without raising a TypeError.
In `@src/sentry/workflow_engine/processors/detector.py`:
- Around line 298-301: The code currently calls PriorityLevel(new_status) inside
the else branch of the detector logic which will raise ValueError when
new_status is DetectorPriorityLevel.OK because OK (0) has no PriorityLevel
equivalent; update the logic in the code that calls
build_occurrence_and_event_data (the call shown) to first check for
DetectorPriorityLevel.OK and handle it explicitly (e.g., skip conversion or map
OK to a specific PriorityLevel member) or implement an explicit mapping function
from DetectorPriorityLevel to PriorityLevel and use that instead of directly
constructing PriorityLevel(new_status); ensure you reference the
DetectorPriorityLevel.OK case and replace the direct PriorityLevel(new_status)
usage with the safe branch or mapper before passing the value into
build_occurrence_and_event_data.
In `@tests/sentry/workflow_engine/processors/test_detector.py`:
- Around line 190-192: The test uses
build_mock_occurrence_and_event(detector.detector_handler, "group_2", 6,
PriorityLevel.HIGH) but the input data sets "group_2": 10, so update the
expected occurrence value to 10 to match the input; locate the call to
build_mock_occurrence_and_event in the test (using detector.detector_handler and
"group_2") and change the third argument from 6 to 10 so the expectation
accurately reflects the test data and avoids future fragility if the mock starts
honoring the value parameter.
🧹 Nitpick comments (1)
tests/sentry/workflow_engine/processors/test_detector.py (1)
45-50: Preferstart()/stop()over manual__enter__/__exit__for mock patchers.Using
self.sm_comp_patcher.start()insetUpandself.sm_comp_patcher.stop()intearDownis the idiomatic pattern for unittest patchers and avoids manually passingNoneargs to__exit__.♻️ Suggested refactor
def setUp(self): super().setUp() self.sm_comp_patcher = mock.patch.object( StatusChangeMessage, "__eq__", status_change_comparator ) - self.sm_comp_patcher.__enter__() + self.sm_comp_patcher.start() ... def tearDown(self): super().tearDown() - self.sm_comp_patcher.__exit__(None, None, None) + self.sm_comp_patcher.stop()Also applies to: 82-84
| # TODO: This will be a stateful detector when we build that abstraction | ||
| class MetricAlertDetectorHandler(DetectorHandler[QuerySubscriptionUpdate]): | ||
| def evaluate( | ||
| self, data_packet: DataPacket[QuerySubscriptionUpdate] | ||
| ) -> list[DetectorEvaluationResult]: | ||
| # TODO: Implement | ||
| return [] | ||
| class MetricAlertDetectorHandler(StatefulDetectorHandler[QuerySubscriptionUpdate]): | ||
| pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, search for MetricAlertDetectorHandler and metric_alert_fire usage
echo "=== Searching for MetricAlertDetectorHandler usage ==="
rg -n "MetricAlertDetectorHandler" --type py -C 2
echo -e "\n=== Searching for metric_alert_fire usage ==="
rg -n "metric_alert_fire" --type py -C 2Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 1129
🏁 Script executed:
#!/bin/bash
# Check the StatefulDetectorHandler base class to understand its abstract methods
echo "=== Finding StatefulDetectorHandler definition ==="
rg -n "class StatefulDetectorHandler" --type py -A 30
echo -e "\n=== Looking for abstract methods in StatefulDetectorHandler ==="
rg -n "@abstractmethod\|def counter_names\|def get_dedupe_value\|def get_group_key_values\|def build_occurrence_and_event_data" --type py -B 2 -A 3Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 2988
🏁 Script executed:
#!/bin/bash
# Examine the full grouptype.py file to see the complete context
echo "=== Full grouptype.py file ==="
cat -n src/sentry/incidents/grouptype.py | head -50Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 1378
🏁 Script executed:
#!/bin/bash
# Search for where detector_handler is actually used/instantiated
echo "=== Searching for detector_handler usage/instantiation ==="
rg -n "\.detector_handler\(|detector_handler(" --type py -B 2 -A 3
echo -e "\n=== Searching for Detector class and how it uses detector_handler ==="
rg -n "class Detector" --type py -A 50 | head -80Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 8103
🏁 Script executed:
#!/bin/bash
# Search for where Detector model instantiates group_type.detector_handler
echo "=== Looking for detector instantiation patterns ==="
rg -n "group_type\.detector_handler\|self\.detector_handler\|detector\.detector_handler" --type py -B 2 -A 3Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Check if there's any code that actually uses MetricAlertFire/metric_alert_fire
echo "=== Searching for MetricAlertFire or metric_alert_fire usage ==="
rg -n "MetricAlertFire\|metric_alert_fire" --type py -C 3Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Search for how detector_handler is accessed/called
echo "=== Searching for detector_handler property access ==="
rg -n "detector_handler" --type py -B 2 -A 2
echo -e "\n=== Looking for Detector model definition ==="
rg -n "class Detector" --type py -B 2 -A 30 | head -100Repository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 14602
🏁 Script executed:
#!/bin/bash
# Check if there's any indication this GroupType is currently enabled/used
echo "=== Searching for GroupType registry or usage ==="
rg -n "MetricAlertFire\|8001\|metric_alert_fire" --type pyRepository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Verify the exact instantiation code to be 100% certain
echo "=== Detector.detector_handler property instantiation ==="
sed -n '63,86p' src/sentry/workflow_engine/models/detector.py
echo -e "\n=== StatefulDetectorHandler abstract methods ==="
sed -n '135,175p' src/sentry/workflow_engine/processors/detector.pyRepository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 2511
Stale TODO and incomplete abstract method implementations.
Line 10: The TODO comment states "This will be a stateful detector when we build that abstraction" — but MetricAlertDetectorHandler already extends StatefulDetectorHandler. This comment is misleading and should be updated or removed.
More critically, StatefulDetectorHandler declares four abstract methods: counter_names, get_dedupe_value, get_group_key_values, and build_occurrence_and_event_data. Since MetricAlertDetectorHandler contains only pass, attempting to instantiate it (which occurs in Detector.detector_handler via group_type.detector_handler(self)) will raise TypeError: Can't instantiate abstract class MetricAlertDetectorHandler with abstract methods [...].
While MetricAlertFire is not currently wired to any detectors (as noted in the comment on line 15), this incomplete code should be addressed before merge. Implement the required abstract methods or keep MetricAlertDetectorHandler as a concrete DetectorHandler subclass until the abstraction is ready.
🤖 Prompt for AI Agents
In `@src/sentry/incidents/grouptype.py` around lines 10 - 12, Remove or update the
misleading TODO and make MetricAlertDetectorHandler concrete by either (A)
implementing the four abstract methods declared on StatefulDetectorHandler —
counter_names, get_dedupe_value, get_group_key_values, and
build_occurrence_and_event_data — with appropriate logic or stubs that delegate
to the existing MetricAlertFire behavior, or (B) if the stateful abstraction
isn't ready, change MetricAlertDetectorHandler to inherit from the concrete
DetectorHandler instead of StatefulDetectorHandler so it no longer requires
those abstract methods; ensure Detector.detector_handler can instantiate
MetricAlertDetectorHandler without raising a TypeError.
| else: | ||
| result, event_data = self.build_occurrence_and_event_data( | ||
| group_key, value, PriorityLevel(new_status) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the enum values of DetectorPriorityLevel and PriorityLevel
echo "=== DetectorPriorityLevel ==="
rg -n "class DetectorPriorityLevel" -A 20 --type py
echo -e "\n=== PriorityLevel ==="
rg -n "class PriorityLevel" -A 20 --type pyRepository: admin-coderabbit/coderabbit-sentry-mirror
Length of output: 5279
Fix unsafe enum conversion PriorityLevel(new_status) — DetectorPriorityLevel.OK has no PriorityLevel equivalent.
DetectorPriorityLevel includes an OK member (value 0) that does not exist in PriorityLevel, which only defines LOW (25), MEDIUM (50), and HIGH (75). Passing DetectorPriorityLevel.OK to PriorityLevel() will raise ValueError at runtime.
Handle the OK case separately or map it explicitly to a valid PriorityLevel member.
🤖 Prompt for AI Agents
In `@src/sentry/workflow_engine/processors/detector.py` around lines 298 - 301,
The code currently calls PriorityLevel(new_status) inside the else branch of the
detector logic which will raise ValueError when new_status is
DetectorPriorityLevel.OK because OK (0) has no PriorityLevel equivalent; update
the logic in the code that calls build_occurrence_and_event_data (the call
shown) to first check for DetectorPriorityLevel.OK and handle it explicitly
(e.g., skip conversion or map OK to a specific PriorityLevel member) or
implement an explicit mapping function from DetectorPriorityLevel to
PriorityLevel and use that instead of directly constructing
PriorityLevel(new_status); ensure you reference the DetectorPriorityLevel.OK
case and replace the direct PriorityLevel(new_status) usage with the safe branch
or mapper before passing the value into build_occurrence_and_event_data.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | ||
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | ||
| ) |
There was a problem hiding this comment.
Incorrect value for group_2's expected occurrence.
Line 177 sets "group_2": 10 in the input data, but the expected occurrence here is built with value=6. Currently this doesn't matter because build_mock_occurrence_and_event ignores the value parameter, but it makes the test misleading and fragile — if the mock is ever updated to use value, this will silently produce the wrong expectation.
Suggested fix
occurrence_2, event_data_2 = build_mock_occurrence_and_event(
- detector.detector_handler, "group_2", 6, PriorityLevel.HIGH
+ detector.detector_handler, "group_2", 10, PriorityLevel.HIGH
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 6, PriorityLevel.HIGH | |
| ) | |
| occurrence_2, event_data_2 = build_mock_occurrence_and_event( | |
| detector.detector_handler, "group_2", 10, PriorityLevel.HIGH | |
| ) |
🤖 Prompt for AI Agents
In `@tests/sentry/workflow_engine/processors/test_detector.py` around lines 190 -
192, The test uses build_mock_occurrence_and_event(detector.detector_handler,
"group_2", 6, PriorityLevel.HIGH) but the input data sets "group_2": 10, so
update the expected occurrence value to 10 to match the input; locate the call
to build_mock_occurrence_and_event in the test (using detector.detector_handler
and "group_2") and change the third argument from 6 to 10 so the expectation
accurately reflects the test data and avoids future fragility if the mock starts
honoring the value parameter.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit